Do we need to take care of any unfinished Span?

Can you help me understanding the closing logic of opentelemetry here? Thank you

I am looking into this

@tanjialiang Addressed by ending any unended spans at the time of ending trace.

Let's remove these headers to presto-open-telemetry. Check my other comment

Can we use the header instead of session property to control the flags?

Let's refactor the code a bit so we don't have to use session property or the header explicitly. Check my other comment

Move this to presto-open-telemetry module. Check my other comment

nit

@Nullable
private TracerProvider tracerProvider;

check non null

directly return instead of creating openTelemetry

[Major] this is my major comment.

Similar to many other "handle" classes in our codebase, let's create a handle interface in com.facebook.presto.spi.tracing:

public interface TracerHandle
{
}

Change the signature of getNewTracer to

Tracer getNewTracer(TracerHandle handle);

Create a new method of TracerProvider:

/**
 * The method returns a function to take a set of HTTP headers and generate the tracer handle
 */
Function<Set<String>, TracerHandle> getHandleGenerator();

In your OpenTelemetry implementation, you should have

public class OpenTelemetryTracerHandle
        implements TracerHandle
{
    private final String traceToken;
    private final String contextPropagator;
    private final String propagatedContext;
    private final String baggage;

    ... 
}

In your OpenTelemetryTracerProvider, you should have

@Override
public Tracer getNewTracer(TracerHandle handle)
{
    OpenTelemetryTracerHandle tracerHandle = (TracerHandle) handle;
    return new OpenTelemetryTracer(tracerHandle.getXXX, ...);
}

In your getHandleGenerator implementation, move all the logics you have in presto-main here so we can hide all those headers and specialized extraction methods.

add javadoc

add javadoc

I know it's a bit anti pattern; but in order to keep the logic so we don't pollute the traceToken below. Check my comment below.

Add a new branch here: else if (trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN)) != null) or something like that. And inside the body, we can either create a SimpleTracerProvider and get the token or directly assign traceToken from PRESTO_TRACE_TOKEN

updated trace token to replicate similar logic as before

Use immutableMap builder

nit: it's not critical but good to follow:

we don't usually name a function or var with word "Map"; instead, we use getRequestHeaders. Same for the variable headerMap --> headers. And everything else in the PR.

nit: requestHeaders

remove else; it's redundant

remove else; it's redundant

remove else; it's redundant

this is not a generic error; check my other comment

two options:

• you can create your own error code class within the module with a brand new base code; check any other implementation of ErrorCodeSupplier; or
• you can just throw runtime error or put generic code to be a catch-all to save the effort, which is not recommended but fine

nit: ImmutableMap<String, String> -> Map<String, String>

nit: Optional.of(tunnelTraceId)

it's not null already

nit: ImmutableMap<String, String> -> Map<String, String>

comment not addressed? https://github.com/prestodb/presto/pull/18534/files#r1035527320

We need a base so we don't collide with other error codes. Find a unique one closest to an existing one. I assume it's starting with 0x05... something

I'm not expert in open telemetry but are we sure the logic is correct by modifying static variables?

I believe it should be fine. If we have a different context propagator, we need to rebuild the open telemetry instance for all tracers that may exist.

Can we delete this class from this file?

Done